Migrate to ESM #324
Migrate to ESM #324
Conversation
config/config_DEPRECATED.ts
Outdated
| path && logger.debug(`Loading config from ${path}`); | ||
| if (path) { | ||
| logger.debug(`Loading config from ${path}`); | ||
| } |
There was a problem hiding this comment.
Had to update these to satisfy new eslint rules
| }, | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
I am admittedly not 100% sure why this one was necessary, but it's just a test so I think that's ok. Claude really struggled with getting mocking to work to test behavior on windows
There was a problem hiding this comment.
Wait how did you test this on Windows?
| * @returns {Promise} - Returns _default_ exported content if ESM, or exported module content if CJS. | ||
| */ | ||
| async function dynamicImport(filePath: string): Promise<any> { | ||
| if (semver.gte(process.version, '13.2.0')) { |
There was a problem hiding this comment.
We should test this but I don't think any of this code was necessary because we no longer support this node version
There was a problem hiding this comment.
Yeah this is ok to remove if this is going to be a major release anyways
utils/PortManagerServer.ts
Outdated
| } | ||
| }; | ||
|
|
||
| async close(): Promise<void> { |
There was a problem hiding this comment.
Adding this helper made the PortManagerServer a lot easier to test (failing tests here were why I started this process in the first place)
camden11
requested review from
brandenrodgers,
chiragchadha1 and
joe-yeager
as code owners
| "yargs": "^17.7.2" | ||
| }, | ||
| "exports": { | ||
| "./*": "./lib/*.js", |
There was a problem hiding this comment.
Should we add a line here for ./*/*? Or is that what you're doing in a more targeted way with the new cms export?
There was a problem hiding this comment.
I figured it'd be better to be explicit about it (we do the same thing with the config dir) but open to changing it too
| : this.fieldOptions; | ||
|
|
||
| const convertFieldsProcess = fork( | ||
| path.join(__dirname, './processFieldsJs.js'), |
There was a problem hiding this comment.
Apparently __dirname is not supported in ESM. Cursor says to use this instead
const __dirname = dirname(fileURLToPath(import.meta.url));There was a problem hiding this comment.
Oh good catch. Just updated!
brandenrodgers
left a comment
brandenrodgers
left a comment
There was a problem hiding this comment.
this lgtm 👍 we can test more once it's out on a beta release alongside the parsing lib updates
Description and Context
This PR migrates local-dev-lib to use ESM rather than CommonJS. It does a few other things to make this possible
The vast majority of the changes in this PR are just updates to import paths (for ESM) and changes from vite to jest. I tried to call out anything beyond that in the comments
TODO
Who to Notify
@brandenrodgers @chiragchadha1 @joe-yeager